New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[extractor/goplay] Fix GoPlay downloads #6654
Conversation
I tested it using: https://www.goplay.be/video/assisen/assisen-s1/assisen-s1-aflevering-6#autoplay
It only downloaded 22min 44seconds (full episode is 43:44) |
As far as I can tell, you just need to move |
More info about the 'part' splitting here #6235 (comment) |
@pukkandan Yes, that would work in this case, but this would also change it for all other extractors that use that code. Is merging periods something that should happen everywhere? It might make sense, but I don't know if there are cases where you'd like to have the periods separately. I also don't know how this would work for livestreams. In short: yes, it would work for GoPlay and other cases, but I don't know enough about MPD manifests or yt-dlp to say if you're missing something. But I'd be happy to move it to |
Strange. Can you see how many fragments it downloads? 22:44 is the length of the first period, according to the manifest. For me it shows |
I see. Maybe there is something strange when you concatenate these streams. I only have VLC, for which it works, and ffmpeg also doesn't show any issues with the file. I've now changed the code to skip the |
[GoPlay] Logging in |
That might be ad-related. With my Dutch IP address I don't get any ads. With a Belgian IP the manifest did include some ads, which are served differently and do not have the The new version should skip any fragment that doesn't have a |
I ran the file trough avidemux and put the correct end time and saved it. Now opening the file in VLC is showing 41:09 and no more artificats. Is there a way to give it the correct time length in yt-dlp? I guess its some sort of metadata? |
@pukkandan pointed me to the YoutubeDL.py file And after downloadin the file it did the fixup and now the correct timestamp is shown also the artifacts are gone |
So instead of omitting the duplicate init fragments, we can just run the FixupDuplicateMoov postprocessor. The above patch was for testing purposes though, and is not viable since it would impact all DASH downloads (e.g. regular youtube videos, etc). Maybe there could be a new format field added for I think the problem may have been caused by the dash init fragments containing duration metadata and other information that interferes with playback. Ideally the DASH periods would be downloaded as separate videos and then properly concatenated with ffmpeg. But if joining all the periods' fragments into one video and then fixing up with ffmpeg afterward works, then that'll do. |
OK, new approach, based on @bashonly's comment: using |
9a94da0
to
fbd8374
Compare
I'm not on Discord so I haven't followed the discussion there. I'm happy to switch to the first solution if that's the preferred option. However, to me the second option feels cleaner (apart from the method names and actual implementation). Concatenating the fragments list based on the format is messy. It's a hack that works for GoPlay, but it is far from perfect:
So if this is the preferred option, I think it would be best to do the merging of the fragments in the GoPlay extractor so it doesn't affect anything else. On the contrary, the second option has several advantages:
I'll change the method name so we can see how that looks. If that doesn't help, we can always go back to the first option. |
I pulled your version and tested it To be honest, now it gives back a playlist of 3 parts with each its own 1000k,1500k,2300k,4300k output... and is a hassle to download a certain format. I prefered the old method where it merged the fragments. The only thing that needed to happen was run the 'ffmpeg' fix. (see #6654 (comment)). Not sure what the correct aproach is here.. |
For the formats, you could use something like But sure, it might be useful to merge the formats in this case. Perhaps something like this? First parse the MPD manifest with the correct period structure, then make an extractor-specific decision on whether and how to merge the formats. |
dash.py: FD_NAME = 'dashsegments' so I guess the check should be
|
Yes, I suppose so. But then it also didn't run for |
Yea I am wondering, I can't find any reference to |
Thank you for all the effort; Tried with a few different videos and seems to be working well |
@pukkandan If there's anything I can do to make this easier to review, please let me know! |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
@pukkandan Thanks for the review. I've made the changes you suggested. An open issue is the support for subtitles in multi-period streams. There was a warning for this (triggered on streams with more than one period and at least one subtitle), which I have now removed. But maybe it should be put back. The reason this warning was here is that the combination of multi-period streams with subtitles might not yet work. For the audio/video streams, the fragments lists are concatenated to download one big stream. For subtitles, there is currently no such thing: they will show up as separate files. I have not yet found a streaming URL with multi-period subtitles where I can test this, so I have no idea what will happen. Some MPD manifests seem to list the same subtitle file in each period and rely on the time codes to work over multiple periods. In the current implementation, I think the single subtitle file might work with the concatenated video. But I am not entirely sure, hence the warning. If the subtitles are split over multiple files, then I think you'd have to download all of them, and I suspect you may have to correct the timestamps as well. This is speculation, I have no examples. At least the subtitles will now all show up in the list. Maybe it would be good to keep the warning that multi-period streams with subtitles are untested, so people can submit bug reports and examples to find these edge cases in the wild? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pls avoid force-pushing
A temporary failure in one of the tests, but otherwise it should be fine. |
@pukkandan Is there anything else that needs to be done before this can be merged? |
Authored by: alard Closes yt-dlp#6235
IMPORTANT: PRs without the template will be CLOSED
Description of your pull request and other information
This PR fixes two issues with the GoPlay extractor for recent broadcasts:
Point 1 is fairly straightforward: the metadata JSON provides a new
ssai
field that points to themanifest.mpd
. See also this recent change in the Retrospect Kodi addon.Point 2 is more difficult. The GoPlay manifests split programmes in multiple periods (e.g., three parts with ads in between). The current MPD implementation ignores the period division and lists all streams in a single list of formats. As a result, there are usually three audio and three video streams with the highest quality. By default, yt-dlp downloads only one audio and one video stream: this is only one period of the programme, and sometimes the audio does not match the video.
This PR fixes this issue at the GoPlay extractor level, by merging the
fragments
lists of the same format in different periods. The end result is a formats list with only one stream for each format that combines all periods.It might be better to solve this at the MPD download level instead of in the GoPlay extractor. I assume that this is a general problem for any MPD manifest with multiple periods. But that is a separate issue and might require larger changes in how yt-dlp works.
Fixes #6235.
Template
Before submitting a pull request make sure you have:
In order to be accepted and merged into yt-dlp each piece of code must be in public domain or released under Unlicense. Check all of the following options that apply:
What is the purpose of your pull request?